Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH] Implemented a component for uploading and previewing the data table #38

Merged
merged 16 commits into from
Feb 19, 2025

Conversation

rmanaem
Copy link
Contributor

@rmanaem rmanaem commented Feb 14, 2025

Checklist

This section is for the PR reviewer

  • PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF], [MODEL], [DOC]) (see our Contributing Guidelines for more info)
  • PR has a label for the release changelog or skip-release (to be applied by maintainers only)
  • PR links to GitHub issue with mention Closes #XXXX
  • Tests pass
  • Checks pass

For new features:

  • Tests have been added

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

Copy link

netlify bot commented Feb 14, 2025

Deploy Preview for staging-annotation ready!

Name Link
🔨 Latest commit 5087438
🔍 Latest deploy log https://app.netlify.com/sites/staging-annotation/deploys/67b64213479eca00088c2bb8
😎 Deploy Preview https://deploy-preview-38--staging-annotation.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rmanaem rmanaem added the pr-minor Non-breaking feature or enhancement, will increment minor version (0.+1.0) label Feb 14, 2025
@rmanaem rmanaem requested a review from surchs February 14, 2025 20:42
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 77.90698% with 19 lines in your changes missing coverage. Please review.

Project coverage is 80.48%. Comparing base (3760165) to head (5087438).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/components/DataTablePreview.tsx 68.18% 7 Missing ⚠️
src/components/UploadCard.tsx 76.00% 6 Missing ⚠️
src/stores/data.ts 81.81% 4 Missing ⚠️
src/components/Upload.tsx 75.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #38      +/-   ##
==========================================
- Coverage   88.09%   80.48%   -7.61%     
==========================================
  Files          12       17       +5     
  Lines          42      123      +81     
  Branches        7       17      +10     
==========================================
+ Hits           37       99      +62     
- Misses          5       24      +19     
Flag Coverage Δ
tests 80.48% <77.90%> (-7.61%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@surchs surchs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @rmanaem, thanks a lot for the PR. Looks very good, really liking the UI!

I have to main asks:

  1. Let's split responsibility for upload and data between the components. Ideally this also means lifting the responsibility for parsing and restructuring the uploaded data out of the components and either into the store or into a dedicate function because that should facilitate testing a whole lot
  2. There is currently a bug where the datatable is not visible after navigating away from the upload page. I believe this bug will be both very obvious and very easy to fix once we do the concern split. But to be safe, let's include a test for it

@rmanaem rmanaem requested a review from surchs February 18, 2025 21:41
@rmanaem
Copy link
Contributor Author

rmanaem commented Feb 18, 2025

Hey @surchs,
Just pushed the recent changes.
I'm gonna resolve the convos from the previous review so we can track the one from the new review easier.

Copy link

@surchs surchs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rmanaem, this looks great. Left one comment you wanted to look at and then 🧑‍🍳

@rmanaem rmanaem merged commit 34f7aee into main Feb 19, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-minor Non-breaking feature or enhancement, will increment minor version (0.+1.0)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upload datatable component
2 participants